-
Notifications
You must be signed in to change notification settings - Fork 2k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat(tabs): keep connection for tabs-[INS-4778] #8266
base: feat/multiple-tab
Are you sure you want to change the base?
Conversation
00aee7b
to
404d75e
Compare
// Close all websocket connections when the active environment changes | ||
useEffect(() => { | ||
return () => { | ||
window.main.webSocket.closeAll(); | ||
window.main.grpc.closeAll(); | ||
}; | ||
}, [activeEnvironment?._id]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This a trade-off; Previously, we closed all connections when the active environment changed to avoid users not knowing which URL is currently used to connect if included some env.
Now, based on multiple tabs, we may have multiple WebSocket or grpc tabs from different workspaces, and we want to keep all the connections when the corresponding tab exists. When a workspace's active environment changes, finding which requests are affected and closing them will introduce more complexity.
So I just delete it and only close connection when tab close.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
discuss with @ihexxa. I will try to emit an event through the event bus when modifying the active environment. So that we could get the workspaceId
and filter the connections under the workspace then close them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated in this commit: f367b65
6469466
to
43fb92a
Compare
e0e0b04
to
cbbc8c2
Compare
4e06cac
to
2dc6493
Compare
cbbc8c2
to
d0e9218
Compare
73e9a24
to
ff9f748
Compare
8303eaa
to
db8f798
Compare
export const useCloseConnection = () => { | ||
// close websocket&grpc&SSE connections | ||
const closeConnection = useCallback((_: string, ids: 'all' | string[]) => { | ||
if (ids === 'all') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If user just closse all tabs in one org, will this also close connections from other organizations?
import { useInsomniaTabContext } from '../context/app/insomnia-tab-context'; | ||
import uiEventBus, { UIEventType } from '../eventBus'; | ||
|
||
export const useCloseConnection = () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: it would be great if we can add short description for new hooks' usages and goal.
packages/insomnia/src/ui/eventBus.ts
Outdated
export enum UIEventType { | ||
CLOSE_TAB = 'closeTab', | ||
CHANGE_AVTIVE_ENV = 'changeActiveEnv', | ||
} | ||
class EventBus { | ||
private events: Record<UIEventType, EventHandler[]> = { | ||
[UIEventType.CLOSE_TAB]: [], | ||
[UIEventType.CHANGE_AVTIVE_ENV]: [], | ||
}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should all ideally be modelled in the main context for consistency of implementation and in order to minimize bugs and surprises due to the deviation in implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These logics will only run in the renderer process, so I think it is unnecessary to pass the event to the main process, to avoid the communication and IPC function code.
In my opinion, we could clarify the scope of the code according to the scenario. When the code is only effective in the rendering process, we need to try to avoid introducing it into the main process.
This eventbus
class can be a common approach when we want to communicate between different components and not need the main process.
import { useInsomniaTabContext } from '../context/app/insomnia-tab-context'; | ||
import uiEventBus, { UIEventType } from '../eventBus'; | ||
|
||
// this hook is use for control when to close connections(websocket & SSE & grpc stream & graphql subscription) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a react hook is a mechanism to wrap react features into a observable function for reuse.
close all connections appears to be a fire and forget behavior.
why introduce all this complexity?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This scenario is indeed a bit complicated, we can't simply know when to close a connection, such as clicking a button and then closing the connection in the callback.
The connection may be closed because the user closed the tab manually or the user deleted the request or workspace. Using eventbus can decouple the connection control logic from the tab. Also I think abstract these logic into this custom hook could make the debug.tsx
component simpler to read, and put the connection control logic together for easy maintenance.
// Close all websocket connections when the active environment changes | ||
useEffect(() => { | ||
return () => { | ||
window.main.webSocket.closeAll(); | ||
window.main.grpc.closeAll(); | ||
}; | ||
}, [activeEnvironment?._id]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this use effect was clear about what happened and why, we close all connections when active env changes, the new hook is unclear about both of these questions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Under the multiple-tabs scenario, when we switch to another tab that belongs to another workspace, activeEnvironment
also changes and runs the logic in this useEffect. This is not as expected, we want to keep the connection when the tab exists, so we have to use another approach to implement it.
0b5f7f3
to
8f4715d
Compare
Changes: